Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate VisualizationEmbed to React #4364

Merged
merged 8 commits into from
Dec 24, 2019
Merged

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented Nov 16, 2019

What type of PR is this? (check all applicable)

  • Refactor

Description

Migration of VisualizationEmbed to React

All as is, with one small difference in the "Open in Redash" icon, I changed the icon to "External Link" instead of "Link".

I also changed the Refresh button because it doesn't seem to work in the Visualization Embed context, lmk if I should handle that differently 🙂

Related Tickets & Documents

#3071

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

visualization-embed

@gabrieldutra gabrieldutra self-assigned this Nov 16, 2019
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to play around with it, but noticed:

alt="Redash Logo"
style={{ height: '24px', verticalAlign: 'text-bottom' }}
/>
<VisualizationName visualization={visualization} />{' '}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we no longer need the react2angular definition of this component...

@arikfr arikfr merged commit 7223f60 into master Dec 24, 2019
@arikfr arikfr deleted the react-visualization-embed branch December 24, 2019 08:21
@arikfr
Copy link
Member

arikfr commented Dec 24, 2019

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants